Conversation
…ned result when nothing has changed
📝 WalkthroughWalkthroughA new test case was added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit f0f4305
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
652-660: This test can pass even ifcombinerecomputes.At Line 659,
toBealone doesn’t prove the cached branch was hit, becausereplaceEqualDeepcan still return the previous reference after a recomputation with equal output. Please also assert thatcombineis not called again on the second pass.Proposed test hardening
const [raw1, getCombined1] = observer.getOptimisticResult(queries, combine) const combined1 = getCombined1(raw1) + const callsAfterFirst = combine.mock.calls.length const [raw2, getCombined2] = observer.getOptimisticResult(queries, combine) const combined2 = getCombined2(raw2) // Same combine, same queries → cached result returned + expect(combine.mock.calls.length).toBe(callsAfterFirst) expect(combined1).toBe(combined2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/queriesObserver.test.tsx` around lines 652 - 660, The test currently only asserts reference equality (expect(combined1).toBe(combined2)) which can pass even if combine recomputed; update the test to ensure combine is not invoked on the second call by using a spy/mocked function for combine (e.g., replace the plain combine with a jest.fn or equivalent) and assert its call count before and after the second getCombined call when calling observer.getOptimisticResult(queries, combine) and getCombined2(raw2); specifically reference the combine function and observer.getOptimisticResult to locate where to wrap combine and add an assertion like expect(combine).not.toHaveBeenCalledTimes/incremented on the second pass in addition to the existing toBe assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/query-core/src/__tests__/queriesObserver.test.tsx`:
- Around line 652-660: The test currently only asserts reference equality
(expect(combined1).toBe(combined2)) which can pass even if combine recomputed;
update the test to ensure combine is not invoked on the second call by using a
spy/mocked function for combine (e.g., replace the plain combine with a jest.fn
or equivalent) and assert its call count before and after the second getCombined
call when calling observer.getOptimisticResult(queries, combine) and
getCombined2(raw2); specifically reference the combine function and
observer.getOptimisticResult to locate where to wrap combine and add an
assertion like expect(combine).not.toHaveBeenCalledTimes/incremented on the
second pass in addition to the existing toBe assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e27aa12f-bce0-4c51-acb6-07107b4db704
📒 Files selected for processing (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx
size-limit report 📦
|
🎯 Changes
Add a test to verify that
#combineResultreturns the cached#combinedResultwithout recalculating when none of the conditions (!#combinedResult,#result !== #lastResult,queryHashesChanged,combine !== #lastCombine) are met.This covers the else path of the if block in
#combineResult.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit